Skip to content

Conversation

@AymDev
Copy link

@AymDev AymDev commented Feb 3, 2026

Related to #396 : I added attributes for (most, not all) properties matching the getters of the Location interface, so we can easily map them to Doctrine entity properties as we can already do for latitude & longitude. Let me know what you think 🙂

Summary by CodeRabbit

  • New Features

    • Extended geocoding data capture: bounds (north, south, east, west), street details (number, name) and location fields (locality, postal code, sub-locality, country) are now populated in entities in addition to latitude/longitude.
  • Tests

    • Expanded test coverage and updated test inputs/constants to validate the new geocoding fields and address components.

@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

📝 Walkthrough

Walkthrough

Adds ten new property-level PHP 8 attributes for address/bounds, extends ClassMetadata to hold their ReflectionProperty refs, centralizes attribute→metadata mapping in AttributeDriver, and populates those new fields in the Doctrine ORM GeocodeEntityListener. Tests and fixtures updated accordingly.

Changes

Cohort / File(s) Summary
New Mapping Attributes
src/Mapping/Attributes/Country.php, src/Mapping/Attributes/East.php, src/Mapping/Attributes/Locality.php, src/Mapping/Attributes/North.php, src/Mapping/Attributes/PostalCode.php, src/Mapping/Attributes/South.php, src/Mapping/Attributes/StreetName.php, src/Mapping/Attributes/StreetNumber.php, src/Mapping/Attributes/SubLocality.php, src/Mapping/Attributes/West.php
Adds 10 marker PHP 8 attribute classes targeting properties for address components and bounding coordinates.
Mapping Metadata
src/Mapping/ClassMetadata.php
Extends ClassMetadata::__construct with 10 new public readonly ?\ReflectionProperty parameters to store reflections for the new fields.
Attribute Driver
src/Mapping/Driver/AttributeDriver.php
Introduces a PROPERTY_MATRIX mapping and refactors attribute scanning to populate metadata properties via the matrix rather than many individual conditionals.
ORM Listener
src/Doctrine/ORM/GeocodeEntityListener.php
When a geocoding result is found, sets north/south/east/west and address components (streetNumber, streetName, locality, postalCode, subLocality, country) onto entities using metadata ReflectionProperty refs.
Test Fixtures
tests/Functional/Fixtures/Entity/DummyWithProperty.php, tests/Mapping/Driver/Fixtures/Dummy.php
Adds the 10 new public properties with Doctrine Column mappings and corresponding attribute markers for test entities.
Tests
tests/Functional/GeocodeEntityListenerTest.php, tests/Mapping/Driver/AttributeDriverTest.php
Introduces QUERY_BERLIN/QUERY_PARIS constants, updates mocked responses and assertions, and adds checks for the new metadata properties and populated fields.
Tooling
phpstan-baseline.php
Adds an ignoreErrors entry for a constructor parameter type warning related to ClassMetadata/AttributeDriver changes.

Sequence Diagram(s)

sequenceDiagram
    participant Entity as Entity
    participant Driver as AttributeDriver
    participant Metadata as ClassMetadata
    participant Listener as GeocodeEntityListener
    participant Geo as GeocodingService

    Entity->>Driver: inspect properties for mapping attributes
    Driver->>Driver: map attribute -> metadata key (PROPERTY_MATRIX)
    Driver->>Metadata: construct metadata with ReflectionProperty refs
    Listener->>Geo: geocode(address)
    Geo-->>Listener: result (lat,lng,bounds,address components)
    Listener->>Metadata: read ReflectionProperty refs for new fields
    Listener->>Entity: set north/south/east/west and address components
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • norkunas

Poem

🐰
I hop through metadata with a curious leap,
North, South, East, West — secrets I keep.
Streets and numbers, postal and place,
I tuck them in properties, a rabbit's small grace.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding new Doctrine attribute classes for geocoding properties. It directly relates to the primary purpose of the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@AymDev
Copy link
Author

AymDev commented Feb 5, 2026

I tried to fix the CI but I don't really understand what's happening with the tests for Symfony 6.4. I opened an issue for the PHPStan error as it looks like a bug to me: phpstan/phpstan#14067

@norkunas
Copy link
Member

norkunas commented Feb 5, 2026

I am also not sure.
Copilot explained this as:

The job failed because multiple functional tests (such as testPersistForProperty, testPersistForGetter, etc.) in tests/Functional/GeocodeEntityListenerTest.php try to require a generated proxy file (EntityManagerGhost*.php) that doesn’t exist at runtime:

require(/tmp/NyholmBundleTest/.../EntityManagerGhost*.php): Failed to open stream: No such file or directory

This typically happens with Doctrine “lazy ghost object” or “proxy” settings plus recent PHP/Doctrine/Symfony versions that have updated how proxies or lazy objects work.

Solution:

Make sure you’re clearing (dropping) and re-creating the Doctrine schema in both setUp and tearDown. You currently only do this in tearDown.
Check your test kernel configuration: in GeocodeEntityListenerTest.php, in createKernel(), there are options for $orm['enable_lazy_ghost_objects'] and $orm['enable_native_lazy_objects']. These toggle advanced Doctrine features that may not work correctly with all version combinations.
Some combinations of PHP >= 8.4, doctrine/orm >= 3.4, and doctrine/doctrine-bundle >= 2.15.0 enable features that are not fully compatible with Symfony’s current or legacy test container.   Code suggestion: Disable native lazy objects or lazy ghost objects for the test suite when running on high PHP or Doctrine versions.
In GeocodeEntityListenerTest.php, replace:

if (method_exists(Configuration::class, 'setLazyGhostObjectEnabled') && Kernel::VERSION_ID >= 60100 && version_compare($doctrineBundleVersion, '2.8.0', '>=') && version_compare($ormVersion, '3.0', '<=') ) { $orm['enable_lazy_ghost_objects'] = true; } if (\PHP_VERSION_ID >= 80400 && version_compare($doctrineBundleVersion, '2.15.0', '>=') && version_compare($doctrineBundleVersion, '3.1.0', '<') && version_compare($ormVersion, '3.4.0', '>=') ) { $orm['enable_native_lazy_objects'] = true; }

with guarded logic or force both to false for affected combinations:

if (\PHP_VERSION_ID >= 80400 && version_compare($ormVersion, '3.4.0', '>=') ) { $orm['enable_lazy_ghost_objects'] = false; $orm['enable_native_lazy_objects'] = false; }

Or if you want to disable just during tests, override them in your test configs.

Summary: The test failures are due to Doctrine proxy/lazy object settings on recent PHP/Doctrine versions. Disabling those options during tests should resolve the missing file error.

Adds additionnal attributes to be able to automatically map them
to Doctrine entities.
@AymDev AymDev force-pushed the feature/additionnal-doctrine-attributes branch from 5018888 to 5868661 Compare February 9, 2026 09:29
@AymDev
Copy link
Author

AymDev commented Feb 9, 2026

I disabled native lazy objects for Symfony 6.4 and ignored the PHPStan error for now. I hope you're okay with the way I "fixed" the CI 😅

@norkunas
Copy link
Member

norkunas commented Feb 9, 2026

Still fails 😞

@AymDev AymDev force-pushed the feature/additionnal-doctrine-attributes branch 2 times, most recently from 225eda1 to f9d6ab3 Compare February 9, 2026 09:45
@AymDev
Copy link
Author

AymDev commented Feb 9, 2026

Weird, I can't reproduce it, the test suite works locally with PHP 8.4 + Symfony 6.4. I updated by explicitly setting the options to false. If that still doesn't pass, I'll have to try something completely different !

Disables the cache clearing as NyholmBundleTest seems to look at
a different cache directory than the one available during testing.
Also ignore a specific PHPStan error that has been reported as a
bug.
@AymDev AymDev force-pushed the feature/additionnal-doctrine-attributes branch from f9d6ab3 to 4047600 Compare February 10, 2026 14:06
@AymDev
Copy link
Author

AymDev commented Feb 10, 2026

Hello again 🙂
I spent some more time trying to fix this issue and it wasn't about Doctrine at all. During debug I've seen that the kernel cache directory where the container was trying to get the entity manager did not exist (well, there was one, but with a different name/ID). I found SymfonyTest/symfony-bundle-test#94 and decided to disable the cleanup. It should work now

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/Functional/GeocodeEntityListenerTest.php (1)

87-100: ⚠️ Potential issue | 🟠 Major

Change version_compare($ormVersion, '3.0', '<=') to version_compare($ormVersion, '3.0', '<')

In Doctrine ORM 3.0, lazy ghost objects are enabled unconditionally and cannot be disabled. The current condition with <= applies the config to ORM 3.0.0+, but the setting is unnecessary (no-op) for ORM 3.0 and is actually removed from DoctrineBundle 3.0+ entirely. Restrict this configuration to ORM 2.x by using < instead of <= to match the ORM versions where lazy ghosts are optional.

🧹 Nitpick comments (1)
tests/Functional/GeocodeEntityListenerTest.php (1)

334-351: Consider extracting long mock JSON responses to fixture files.

Lines 340 and 346 contain very long inline JSON strings (several KB each), which makes this test file harder to read and maintain. Consider moving these to separate .json fixture files and loading them with file_get_contents(). This would also make it easier to update the mock data if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants